PR 423#423
Conversation
- Add docs/operator-apply-july8-batch.md with ordered apply steps for PRs #380/#405/#408/#409 (RPC hardening, R17, R5, fail-closed tenancy) - Add npm run check:july8-live-batch for post-apply live verification - Extend check-retrieval-owner-migration with fail-closed NULL probe - Refresh stale docs: process-hardening, tenancy review, ingestion workorder, redesign/04-deferred, PR template (36/36 golden eval), migration reconciliation Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1662a0868e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hardening - Rename R17 migration to 20260708170000 (avoids timestamp collision with 20260708160000_retrieval_owner_matches_fail_closed and unsafe repair) - Use transactional CREATE UNIQUE INDEX so db push can apply the full batch; document optional CONCURRENTLY path for busy queues - check:july8-live-batch: requireServerEnv (fail when live keys absent); fail R17 probe when schema_drift_snapshot is unavailable - Refresh operator runbook, schema.sql comment, drift manifest hash Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3138d2e345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address PR #423 review: schema_drift_snapshot only exposes index names, so a failed CONCURRENTLY build can pass a name-only probe. Check the partial-unique definition and reject duplicate open-job inserts. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo
changed the title
…robes Importing check-july8-live-batch.ts from unit tests was executing main(), calling live Supabase RPCs and process.exit(1) in the vitest worker. Match the repo script convention: only run when invoked directly. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ca9b2427
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Use randomUUID per enforcement probe; clean legacy fixed probe rows - Require 23505 violations cite ingestion_jobs_one_open_per_document_uidx - Reject broader R17 predicates (failed/completed/etc.) in definition check - Pin check:july8-live-batch to production via checkSupabaseProjectConfig - Extend unit tests and operator runbook for production-only semantics Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Head branch was modified
📝 WalkthroughWalkthroughThis PR adds the July 8 ingestion batch migration and live verification tooling, updates the operator runbook and related status docs, revises the tenancy review for fail-closed owner matching, and raises the retrieval eval checklist threshold. ChangesJuly 8 batch migration, verification tooling, and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/check-july8-live-batch.test.ts (1)
6-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test case using the real Postgres-reconstructed predicate form.
All cases here use literal
WHERE status IN (...)strings. IfisExpectedR17IndexDef's matcher needs to also acceptstatus = ANY (ARRAY[...])(see companion comment onscripts/check-july8-live-batch.ts), add a test asserting that form is accepted too, since that's closer to whatpg_get_indexdef/schema_drift_snapshotwill actually return.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/check-july8-live-batch.test.ts` around lines 6 - 14, Add a test in isExpectedR17IndexDef’s spec that uses the real Postgres-reconstructed predicate form with status = ANY (ARRAY[...]) instead of only the literal IN clause. Update the test in tests/check-july8-live-batch.test.ts alongside the existing canonical partial unique index case, and assert that isExpectedR17IndexDef accepts the ARRAY form while normalizeIndexDef still normalizes it as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-july8-live-batch.ts`:
- Around line 28-51: The R17 index matcher in isExpectedR17IndexDef only
recognizes status IN (...) and will reject a valid partial index rendered by
pg_get_indexdef as status = ANY (ARRAY[...]). Update the parsing in that helper
to accept both IN and ANY(array) forms while still requiring the same
pending/processing pair and excluding broader predicates, and add tests around
schema_drift_snapshot() output to cover the ANY rendering.
- Around line 148-229: findR17ProbeDocument currently stops after a single
200-row fetch, so it can miss a usable document and still race with ingestion
activity; update the R17 probe flow to keep scanning through pages until a
document without open jobs is found. Use the existing findR17ProbeDocument and
checkR17IndexEnforcement helpers, and make the insert path retry or re-select a
new document when a unique-violation/open-job race occurs instead of failing
immediately. Ensure cleanupLegacyR17ProbeRows still runs before each attempt so
stale probe rows do not bias the scan.
---
Nitpick comments:
In `@tests/check-july8-live-batch.test.ts`:
- Around line 6-14: Add a test in isExpectedR17IndexDef’s spec that uses the
real Postgres-reconstructed predicate form with status = ANY (ARRAY[...])
instead of only the literal IN clause. Update the test in
tests/check-july8-live-batch.test.ts alongside the existing canonical partial
unique index case, and assert that isExpectedR17IndexDef accepts the ARRAY form
while normalizeIndexDef still normalizes it as expected.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b434e46f-9b22-48de-a387-e1bb006ba712
📒 Files selected for processing (15)
.github/pull_request_template.mddocs/ingestion-concurrency-fix-workorder.mddocs/operator-apply-july8-batch.mddocs/process-hardening.mddocs/redesign/04-deferred.mddocs/supabase-migration-reconciliation.mddocs/tenancy-defense-in-depth-review.mdpackage.jsonscripts/check-july8-live-batch.tsscripts/check-retrieval-owner-migration.tssupabase/drift-manifest.jsonsupabase/migrations/20260708160000_ingestion_jobs_one_open_per_document.sqlsupabase/migrations/20260708170000_ingestion_jobs_one_open_per_document.sqlsupabase/schema.sqltests/check-july8-live-batch.test.ts
💤 Files with no reviewable changes (1)
- supabase/migrations/20260708160000_ingestion_jobs_one_open_per_document.sql
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10689cc56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
- Tag probe rows with stage july8-live-batch-probe for mark-and-sweep - Check delete errors in finally and unexpected-duplicate paths - Sweep stranded probe rows before each attempt and verify none remain - Document probe cleanup semantics in operator runbook Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check-july8-live-batch.ts (1)
44-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject extra boolean predicates in the R17 matcher.
The matcher still accepts broader predicates that contain the expected status list plus another clause, e.g.
status IN ('pending','processing') OR status IS NOT NULL. That violates the “exact pending + processing only” contract.Proposed guard
const whereClause = normalized.slice(whereIdx + " where ".length); + if (/\b(?:and|or)\b/.test(whereClause)) return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-july8-live-batch.ts` around lines 44 - 67, The R17 status matcher is too permissive because it only extracts the status list from `whereClause` and ignores any extra predicates. Update the parsing in the status-matching logic to require the whole clause to be exactly a `status IN (...)` or `status = ANY(...)` expression with only `pending` and `processing`, and reject cases where additional boolean conditions are present. Use the existing matcher logic around `whereClause`, `inMatch`, and `anyMatch` to add a strict guard before returning true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/check-july8-live-batch.ts`:
- Around line 44-67: The R17 status matcher is too permissive because it only
extracts the status list from `whereClause` and ignores any extra predicates.
Update the parsing in the status-matching logic to require the whole clause to
be exactly a `status IN (...)` or `status = ANY(...)` expression with only
`pending` and `processing`, and reject cases where additional boolean conditions
are present. Use the existing matcher logic around `whereClause`, `inMatch`, and
`anyMatch` to add a strict guard before returning true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 680763d2-cfa3-41d4-8964-558787ced9f4
📒 Files selected for processing (3)
docs/operator-apply-july8-batch.mdscripts/check-july8-live-batch.tstests/check-july8-live-batch.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/operator-apply-july8-batch.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/check-july8-live-batch.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f0184510e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Summary
July 8 live-batch closeout: operator runbook, consolidated probes, and hardened R17 enforcement checks.
Latest (
a0d34cd) — fail-closed probe cleanupstage: july8-live-batch-probefor mark-and-sweepdeleteR17ProbeJobIds()throws on Supabase delete errors (finally block + unexpected-duplicate path)assertNoStrandedR17ProbeRows()after successPrior commits on this branch
IN (...)and= ANY (ARRAY[...])pg_get_indexdef forms20260708170000Verification
npm run verify:cheap— 1386 tests passedtests/check-july8-live-batch.test.ts— 8 tests